-
Notifications
You must be signed in to change notification settings - Fork 471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor default config location resolution to use pathlib instead of os #2017
Refactor default config location resolution to use pathlib instead of os #2017
Conversation
snowflake_home = os.path.expanduser( | ||
snowflake_home = pathlib.Path( | ||
os.environ.get("SNOWFLAKE_HOME", "~/.snowflake/"), | ||
) | ||
if os.path.exists(snowflake_home): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious is there a bug related to the previous implementation?
I checked the code on windows and think they are effectively equivalent, backslash will be handled by the path.Pathlib called in SFPlatformDirs constructor.
or it's more about code improvement?
import os
import pathlib
assert pathlib.Path(os.path.expanduser("~/.snowflake/")) == pathlib.Path(str(pathlib.Path("~/.snowflake/").expanduser()))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, I see - I didn't notice that this is handled via pathlib
later on. I think this might be worth adding anyway, as the code becomes less confusing (I honestly thought that there was a bug in there), although the connector works properly.
I've updated the PR title and description.md
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes #2018
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
Use
pathlib
instead ofos
to resolve default config file paths(Optional) PR for stored-proc connector: